Skip to content

Conversation

@jansvoboda11
Copy link
Contributor

@jansvoboda11 jansvoboda11 commented May 7, 2025

The dependency scanner mixes implicitly- and explicitly-built modules. When an implicitly-built module imports an explicitly-built one, we never run the modification time validation checks, resulting in an out-of-date module cache. This PR fixes that by only skipping the modification time validation checks when both the imported module and its importer are built explicitly.

rdar://150230022

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:modules C++20 modules and Clang Header Modules labels May 7, 2025
@llvmbot
Copy link
Member

llvmbot commented May 7, 2025

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Jan Svoboda (jansvoboda11)

Changes

The dependency scanner mixes implicitly- and explicitly-built modules. When an implicitly-built module imports an explicitly-built one, we never run the modification time validation checks, resulting in an out-of-date module cache. This PR fixes that by only skipping the modification time validation checks when both the imported module and its importer are built explicitly.


Full diff: https://github.com/llvm/llvm-project/pull/138920.diff

2 Files Affected:

  • (modified) clang/lib/Serialization/ModuleManager.cpp (+5-3)
  • (added) clang/test/ClangScanDeps/modules-pch-common-stale.c (+77)
diff --git a/clang/lib/Serialization/ModuleManager.cpp b/clang/lib/Serialization/ModuleManager.cpp
index d466ea06301a6..fdde6109ed0f2 100644
--- a/clang/lib/Serialization/ModuleManager.cpp
+++ b/clang/lib/Serialization/ModuleManager.cpp
@@ -110,10 +110,12 @@ ModuleManager::addModule(StringRef FileName, ModuleKind Type,
   // Look for the file entry. This only fails if the expected size or
   // modification time differ.
   OptionalFileEntryRef Entry;
-  const bool IgnoreModTime =
-      (Type == MK_ExplicitModule || Type == MK_PrebuiltModule);
+  bool IgnoreModTime = Type == MK_ExplicitModule || Type == MK_PrebuiltModule;
+  if (ImportedBy)
+    IgnoreModTime &= ImportedBy->Kind == MK_ExplicitModule ||
+                     ImportedBy->Kind == MK_PrebuiltModule;
   if (IgnoreModTime) {
-    // If we're not expecting to pull this file out of the module cache, it
+    // If neither this file nor the importer are in the module cache, this file
     // might have a different mtime due to being moved across filesystems in
     // a distributed build. The size must still match, though. (As must the
     // contents, but we can't check that.)
diff --git a/clang/test/ClangScanDeps/modules-pch-common-stale.c b/clang/test/ClangScanDeps/modules-pch-common-stale.c
new file mode 100644
index 0000000000000..b8c660a314a88
--- /dev/null
+++ b/clang/test/ClangScanDeps/modules-pch-common-stale.c
@@ -0,0 +1,77 @@
+// Test that modifications to a common header (imported from both a PCH and a TU)
+// cause rebuilds of dependent modules imported from the TU on incremental build.
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+//--- module.modulemap
+module mod_common { header "mod_common.h" }
+module mod_tu { header "mod_tu.h" }
+module mod_tu_extra { header "mod_tu_extra.h" }
+
+//--- mod_common.h
+#define MOD_COMMON_MACRO 0
+
+//--- mod_tu.h
+#include "mod_common.h"
+#if MOD_COMMON_MACRO
+#include "mod_tu_extra.h"
+#endif
+
+//--- mod_tu_extra.h
+
+//--- prefix.h
+#include "mod_common.h"
+
+//--- tu.c
+#include "mod_tu.h"
+
+// Clean: scan the PCH.
+// RUN: clang-scan-deps -format experimental-full -o %t/deps_pch.json -- \
+// RUN:     %clang -x c-header %t/prefix.h -o %t/prefix.h.pch -F %t \
+// RUN:     -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache
+
+// Clean: build the PCH.
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name mod_common > %t/mod_common.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --tu-index 0 > %t/pch.rsp
+// RUN: %clang @%t/mod_common.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Clean: scan the TU.
+// RUN: clang-scan-deps -format experimental-full -o %t/deps_tu.json -- \
+// RUN:     %clang -c %t/tu.c -o %t/tu.o -include %t/prefix.h -F %t \
+// RUN:     -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache
+
+// Clean: build the TU.
+// RUN: %deps-to-rsp %t/deps_tu.json --module-name mod_tu > %t/mod_tu.rsp
+// RUN: %deps-to-rsp %t/deps_tu.json --tu-index 0 > %t/tu.rsp
+// RUN: %clang @%t/mod_tu.rsp
+// RUN: %clang @%t/tu.rsp
+
+// Incremental: modify the common module.
+// RUN: echo "#define MOD_COMMON_MACRO 1" > %t/mod_common.h
+
+// Incremental: scan the PCH.
+// RUN: clang-scan-deps -format experimental-full -o %t/deps_pch.json -- \
+// RUN:     %clang -x c-header %t/prefix.h -o %t/prefix.h.pch -F %t \
+// RUN:     -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache
+
+// Incremental: build the PCH.
+// RUN: %deps-to-rsp %t/deps_pch.json --module-name mod_common > %t/mod_common.rsp
+// RUN: %deps-to-rsp %t/deps_pch.json --tu-index 0 > %t/pch.rsp
+// RUN: %clang @%t/mod_common.rsp
+// RUN: %clang @%t/pch.rsp
+
+// Incremental: scan the TU. This needs to invalidate modules imported from the
+//              TU that depend on modules imported from the PCH.
+// RUN: clang-scan-deps -format experimental-full -o %t/deps_tu.json -- \
+// RUN:     %clang -c %t/tu.c -o %t/tu.o -include %t/prefix.h -F %t \
+// RUN:     -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/cache
+
+// Incremental: build the TU.
+// RUN: %deps-to-rsp %t/deps_tu.json --module-name mod_tu_extra > %t/mod_tu_extra.rsp
+// RUN: %deps-to-rsp %t/deps_tu.json --module-name mod_tu > %t/mod_tu.rsp
+// RUN: %deps-to-rsp %t/deps_tu.json --tu-index 0 > %t/tu.rsp
+// RUN: %clang @%t/mod_tu_extra.rsp
+// RUN: %clang @%t/mod_tu.rsp
+// RUN: %clang @%t/tu.rsp

jansvoboda11 added a commit to swiftlang/llvm-project that referenced this pull request May 8, 2025
In addition to the dependency issue described in llvm#138920, not rebuilding an implicit module when its explicitly-built dependency got out of date made us use stale include-tree of the implicitly-built module, which caused file content conflicts and the "file changed during build" error message.

rdar://150230022
Copy link
Member

@cyndyishida cyndyishida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jansvoboda11 jansvoboda11 merged commit 94ae5f9 into llvm:main May 9, 2025
11 checks passed
@jansvoboda11 jansvoboda11 deleted the pch-common-stale branch May 9, 2025 17:32
jansvoboda11 added a commit to swiftlang/llvm-project that referenced this pull request May 9, 2025
jansvoboda11 added a commit to swiftlang/llvm-project that referenced this pull request May 9, 2025
…plicit imports are (llvm#138920)

The dependency scanner mixes implicitly- and explicitly-built modules.
When an implicitly-built module imports an explicitly-built one, we
never run the modification time validation checks, resulting in an
out-of-date module cache. This PR fixes that by only skipping the
modification time validation checks when both the imported module and
its importer are built explicitly.

rdar://150230022
jansvoboda11 added a commit to swiftlang/llvm-project that referenced this pull request May 9, 2025
In addition to the dependency issue described in llvm#138920, not rebuilding an implicit module when its explicitly-built dependency got out of date made us use stale include-tree of the implicitly-built module, which caused file content conflicts and the "file changed during build" error message.

rdar://150230022
cyndyishida pushed a commit to swiftlang/llvm-project that referenced this pull request May 12, 2025
…plicit imports are (llvm#138920)

The dependency scanner mixes implicitly- and explicitly-built modules.
When an implicitly-built module imports an explicitly-built one, we
never run the modification time validation checks, resulting in an
out-of-date module cache. This PR fixes that by only skipping the
modification time validation checks when both the imported module and
its importer are built explicitly.

rdar://150230022
cyndyishida pushed a commit to swiftlang/llvm-project that referenced this pull request May 12, 2025
In addition to the dependency issue described in llvm#138920, not rebuilding an implicit module when its explicitly-built dependency got out of date made us use stale include-tree of the implicitly-built module, which caused file content conflicts and the "file changed during build" error message.

rdar://150230022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:modules C++20 modules and Clang Header Modules clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants